I vote that we either write those checking functions and use them, or remove this until we have them?

Is this a performance thing?

The main use case is for upgrade. We cannot make HashMap stable, so the upgrade code would look like:

  stable var pureHashMap : HashMap.Tree = [];
  flexible let db: HashMap.HashMap = HashMap.unshareUnsafe(pureHashMap);
  system func preupgrade() {
   pureHashMap := HashMap.share();
  };

The db can be very large when upgrade, it's probably too expensive to check for the invariants.Also for the upgrade case, the invariants always hold.

Makes sense.

This is another use case for types that hide internal data but are not object types. (existential of some flavor? @crusso @rossberg ?)

I'm curious how BigMap handles upgrade?

It doesn't handle it at the moment, but similar issues will arise (how to enforce invariants for the serialized representation without rechecking them).

Also, I think this question will arise for each developer, each time they implement such functions, for any canister with data invariants (so, almost all of them).

Perhaps we should add some notes in the design doc for share and unshare that just clarifies this design point (Do we expect these functions to be defensive and re-establish invariants, or to be trusting and assume that they hold, as with your unsafe variant?)

https://github.com/dfinity/motoko-base/blob/master/doc/design.md#classes

Currently reads as follows:

The share/unshare functions of a class need to convert to a type that is designed for stability (potentially, including extensibility) and space efficiency, not for enabling efficient direct operations.

If we follow this guide, it seems like we should be using a vector of key-value pairs for things like BigMap, and for other map-like structures, and shouldn't even include the hash values if they were computed for internal use only.

Indeed, if at all possible, let's make such functions safe. Otherwise you break encapsulation. I doubt that the copying with freeze and thaw are significantly cheaper than a safe alternative.

Also, for polymorphic data structures, these functions need to be parameterised over the share/unshare functions for each type parameter, otherwise you cannot compose them properly.

Is there an else missing here? Otherwise this will trap when empty.

Indeed, if at all possible, let's make such functions safe. Otherwise you break encapsulation. I doubt that the copying with freeze and thaw are significantly cheaper than a safe alternative.

Also, for polymorphic data structures, these functions need to be parameterised over the share/unshare functions for each type parameter, otherwise you cannot compose them properly.